refactor: move AttributeReference and KnownFloatingPointNormalized expression support checks to getSupportLevel#4745
Conversation
peterxcli
changed the title
| override def getSupportLevel(expr: FromUnixTime): SupportLevel = Incompatible(None) | ||
| override def getSupportLevel(expr: FromUnixTime): SupportLevel = { | ||
| if (expr.format == Literal(TimestampFormatter.defaultPattern()) || | ||
| !CometConf.isExprAllowIncompat(getExprConfigName(expr))) { |
There was a problem hiding this comment.
I'm confused about the reference to isExprAllowIncompat here. Typically we would just return Incompatible and then rely on the serde framework checkign this flag.
| // TODO: DataFusion supports only -8334601211038 <= sec <= 8210266876799 | ||
| // https://github.com/apache/datafusion/issues/16594 | ||
| object CometFromUnixTime extends CometExpressionSerde[FromUnixTime] with CodegenDispatchFallback { | ||
| private val unsupportedFormatReason = "Datetime pattern format is unsupported" |
There was a problem hiding this comment.
Could you implement getUnsupportedReasons, referencing unsupportedFormatReason so that this gets generated into the docs?
peterxcli
changed the title
|
@andygrove looks like the above two comments have been addressed in your pr #4847, I have merged the upstream main, please take another looks on the refactor for AttributeReference and KnownFloatingPointNormalized expr. Thanks! |
|
This looks good and reads as a faithful move of the static datatype checks into Two small things, neither blocking:
This review was prepared with the assistance of an LLM (Claude). |
Sorry, ignore this. I'm happy to merge once CI is green. |
2 participants
Which issue does this PR close?
Closes #4673.
Rationale for this change
This finishes the misc scalar serde follow-up for #4673. These serdes still had static support decisions in
convert, even though the dispatcher expects those decisions to come fromgetSupportLevel.Binding-dependent failures and child fallback rollups still stay in
convert.What changes are included in this PR?
Move static support checks into
getSupportLevelfor:AttributeReference: unsupported data types.FromUnixTime: unsupported native datetime format when the incompatible native path is enabled, while keeping the default codegen-dispatch path unchanged.KnownFloatingPointNormalized: unsupported wrapped child data types.The remaining
withFallbackReasoncalls in these files are for child conversion or binding-dependent failures.How are these changes tested?
Added focused coverage in
CometExpressionSuitefor the misc scalar serdegetSupportLeveldecisions.Also ran:
./mvnw test -Pspark-3.5 -Pscala-2.12 -Dtest=none -Dsuites="org.apache.comet.CometExpressionSuite misc scalar serdes report static unsupported cases via getSupportLevel" -Dscalastyle.skip=true